DependencyManagementDependencyRequiresVersion: remove only entries that manage nothing - #8445
Draft
martinfrancois wants to merge 5 commits into
Conversation
The recipe removed every managed dependency without a `<version>`, on the premise that such an entry cannot affect resolution anywhere. Maven does not honor that premise. An entry without a version still manages `scope`, `exclusions`, `optional` and `systemPath` for a dependency versioned elsewhere, and because Maven merges dependency management one entry at a time on the management key rather than field by field, an entry declaring only coordinates hides, rather than inherits from, a same-key entry coming from a parent or an imported BOM. Removing it silently widened effective scope and dropped exclusions. An entry is now removed only when it declares nothing but `groupId` and `artifactId`, both resolve, and no entry for that key can be hidden: the POM is not one that others inherit from or import, its parent's effective management does not cover the key, no ancestor declares dependency management in a profile or is unresolved here, neither the POM nor its profiles import a BOM, and no sibling entry shares the key. The display description and the generated recipes.csv row are updated to match. The recipe is now strictly more conservative, so it also keeps entries that happen to be inert but whose surroundings cannot be established from the POM under review. The one hidden entry it still cannot see is a BOM import of a POM packaged as something other than `pom`, since only the parent relation is recorded on either side. The existing test for issue 1084 is untouched and still passes: a coordinates-only entry in a standalone POM is still removed.
…im commentary The display name promised a version on every entry, which the recipe no longer enforces. Also simplifies the sibling lookup and hoists the recipe into defaults(RecipeSpec).
…ment-versionless-entry-semantics # Conflicts: # rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv
…ess-entry-semantics' into fix/dependency-management-versionless-entry-semantics # Conflicts: # rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv
martinfrancois
force-pushed
the
fix/dependency-management-versionless-entry-semantics
branch
from
August 16, 2026 20:17
a125cad to
7060026
Compare
martinfrancois
marked this pull request as draft
August 17, 2026 08:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested review order: 13 of 52 (Score: 7)
Review first: #8509
What's changed?
On main
DependencyManagementDependencyRequiresVersionremoves every<dependencyManagement>entry with no<version>child. Its description claims such an entry "can't possibly affect dependency resolution anywhere". It can, in two ways:scope,exclusions,optionalandsystemPathfor a dependency versioned somewhere else.groupId:artifactId:type:classifier, not field by field, so an entry declaring only coordinates overrides, rather than inherits from, an entry for the same key in a parent or an imported BOM. Removing it lets that hidden entry take effect.For the child POM described under motivation, main deletes the versionless
guavaentry and the emptied container elements.mvn help:effective-pomwith Maven 3.9.16 then showsguavamoving fromruntimetocompilescope and thejsr305exclusion disappearing. This also occurs in published code:org.openrewrite.maven:rewrite-maven-plugin:5.40.0has three versionless entries with<scope>provided</scope>that main's recipe deletes. Reproduced on v8.87.0, v8.88.3, and main at80d1f1b7.With this change an entry goes only when it declares nothing but
groupIdandartifactIdand nothing the run can see manages those coordinates elsewhere. Management the run cannot see counts as unknown, and unknown is reason to keep. The entry stays when:pom, or another POM in the same run declares it as its parent, so what a consumer manages cannot be established from here.<profile>, or the ancestry is cyclic.groupIdandartifactId, or has coordinates that do not resolve here.The two key comparisons are deliberately different. Against a parent the lookup uses the entry's exact key -
groupId:artifactId:jar, no classifier, passing a null type thatResolvedPomreads asjar. Against siblings it is coarser, ongroupIdandartifactIdonly, which can keep an otherwise removable entry but never removes an entry Maven would have kept.The recipe now removes a strict subset of what it removed before, so its display name, description and
recipes.csvrow are updated to match.What's your motivation?
Recipe:
org.openrewrite.maven.cleanup.DependencyManagementDependencyRequiresVersion.Before
The parent POM manages
guavaat33.4.8-jre. The child POM overrides its scope and exclusion without repeating the version:Actual after the recipe
<!-- dependency entry deleted -->Deleting the entry changes
guavafromruntimetocompilescope and removes thejsr305exclusion.Expected after the recipe
(unchanged)Affected code in real projects
operaton/operatonpom.xml: the parent POM of the Operaton Run distribution modules manages 14 versionless dependencies with<scope>provided</scope>, so that Spring Boot and engine jars already shipped in the Run distribution stay out of the dependency sets the modules copy withmaven-dependency-plugin(includeScope=runtime). The implementation on main deletes all 14 entries because they carry no<version>, returning those dependencies to compile scope and pulling the duplicated jars back into the distribution modules.cibseven/cibsevenpom.xml: the CIB seven Run modules parent POM uses the same pattern, 14 versionless<scope>provided</scope>entries whose own comment states they mark dependencies already present in the Spring Boot ZIP. The implementation on main deletes every one of them, so the scope management is lost and the excluded jars are copied into the distribution again.Anything in particular you'd like reviewers to focus on?
No existing test expectation changed; the test for #1084 still passes as written.
Two cases keep the most entries: a POM whose declared parent this run did not parse (
spring-boot-starter-parent, say) keeps every entry, and a POM importing any BOM keeps every coordinates-only entry, including ones that BOM does not manage. A third limit is inherited - the recipe still does not visit entries under<profiles>.The test file still has no
@DocumentExample.examples.ymlis generated and checked in, and a hand-written entry would probably not match what the generator produces; 14 recipes in this module carry no example today.Have you considered any alternatives or workarounds?
A more precise rule can look the coordinates up in
ResolvedPom.getDependencyManagement(), which already holds entries inherited from parents and imported from BOMs. That list also contains the resolved form of the entry under review and records only the declaredManagedDependencyeach resolved entry came from, not the POM that declared it, so a naive lookup always matches the entry itself and removes nothing; it has to compare against the entry's own declaredManagedDependencyto tell the self match apart. The work stays insidemanagedElsewhere. Say the word and I will make it.Any additional context
Upstream names the test class
ManagedDependencyRequiresVersionTesteven though the recipe class isDependencyManagementDependencyRequiresVersion; left as is. The class goes from 1 to 21 tests, 17 of which fail without the code change. They cover an entry declaring more than coordinates; a coordinates-only entry hiding a parent or BOM entry; unresolvable coordinates; management the run cannot establish (unparsed parent, profile, cyclic ancestry, a consumer that inherits or imports); and a sibling repeating the coordinates. Four pass either way, covering removals the recipe must still perform.This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv